refactor: change from serialising a u64 to i64 - #3065
Conversation
dannycjones
left a comment
dannycjones
left a comment
There was a problem hiding this comment.
Just noticed this too when reviewing some of the row lineage stuff! Thanks for the PR.
I think this change makes sense, although I think we should extend it to ManifestFile type too which should eliminate the TryInto calls.
|
@dannycjones I can do that, however the PR will expand into a lot of places. The For what it's worth I have a draft PR here where I moved everything across: #3068 . If that is preferable, I can close this PR and open that for review. |
I think that's the right approach - to me, we just chose the wrong type here so we should move those things across. |
|
@blackmwk and/or @CTTY, please can you check this is the right direction? I don't want to lead us on a larger change that we shouldn't make. I see that there's a few types even for V2 ( I am proposing to move from |
anoopj
left a comment
anoopj
left a comment
There was a problem hiding this comment.
The code change looks correct for the Avro 0.22 fix.
One question: the in-memory models (ManifestFile.first_row_id and Snapshot.first_row_id ) are still u64, while the manifest-level first_row_id is already i64 and the spec defines it as a signed long.
Is keeping the model as u64 an intentional scope-limit for the Avro upgrade, or would it be cleaner to make the them i64 too so the whole path matches the spec and the manifest-level type?
Not blocking though.
blackmwk
left a comment
blackmwk
left a comment
There was a problem hiding this comment.
Thanks @JosephLenton for this fix, and @dannycjones @anoopj for review! I think this is the right direction to go, the i64 in ser/de types are to maintain compatibility with languages without native 64bit integer implementation. We could still use unsigned integer for in memory structs unless we see some actual problems.
4 participants
Which issue does this PR close?
This is a part of the upgrade to Avro 0.22. We currently serialise the manifest
first_row_idas au64, and this is incorrect and out of spec.From the Iceberg spec:
first_row_idis defined as along- see: https://iceberg.apache.org/spec/#manifest-lists64-bit signed integer- see: https://iceberg.apache.org/spec/#primitive-typesThis is relevant to the Avro 0.22 upgrade because:
in version 0.21, serialising a
u64as a long is allowed. It is silently coerced.in version 0.22, this fails and is rejected.
Closes some of Upgrade to using
apache-avro0.22 #3063I am making this change on it's own to help isolate changes (and to isolate discussion in case this is the wrong place, or changes elsewhere are needed).
What changes are included in this PR?
first_row_idas au64to ani64.Are these changes tested?
AI Disclosure
The code changes and this PR comment is all made by a human.
Claude was used to debug the issue, and as a search. This included in code, and in spec.